Pull request overview

Fixes #947 by releasing pre-acquired stream IDs when requests fail before reaching the in-flight pipeline.

Changes:

• Adds explicit stream-ID reservation cancellation APIs.
• Covers early failures across CQL, graph, continuous, admin, and channel-handler requests.
• Adds regression tests for key failure paths.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

    try {
      return writeCoalescer.writeAndFlush(channel, message);
    } catch (Throwable t) {
      cancelPreAcquireId();
      return channel.newFailedFuture(t);

        DriverExecutionProfile executionProfile =
            Conversions.resolveExecutionProfile(statement, context);
        GraphProtocol graphSubProtocol =
            GraphConversions.resolveGraphSubProtocol(statement, graphSupportChecker, context);
        Message message =

      } finally {
        if (!writeSubmitted) {
          channel.cancelPreAcquireId();
        }

      } finally {
        if (!writeSubmitted) {
          inFlightHandler.cancelPreAcquireId();
        }

        if (!writeSubmitted) {
          if (nodeResponseCallback != null) {
            inFlightCallbacks.remove(nodeResponseCallback);
          }
          channel.cancelPreAcquireId();

Overview

The reservation is made in ChannelSet.next() and consumed only in InFlightHandler.write(); anything that failed between those two points leaked a slot, so getAvailableIds() drifted downward and channels progressively looked saturated. This closes the gap on every early-exit path I can find — the five channel.write(...) call sites, DriverChannel.write() itself, DefaultSession.getChannel() on a closed pooled channel, and the caller-owned reservation in ThrottledAdminRequestHandler.

Verified locally

Built core at bff77b2 on JDK 11 — clean. Ran DriverChannelTest, CqlRequestHandlerTest, DefaultSessionPoolsTest, CqlPrepareHandlerTest, GraphRequestHandlerTest, ContinuousCqlRequestHandlerTest, ReprepareOnUpTest, InFlightHandlerTest, ChannelPoolTest → 127 tests, 0 failures. Surefire enables -ea by default, so StreamIdGenerator's assert available <= maxAvailableIds over-release guard was live for those runs.

I also traced every preAcquireId() producer (ChannelSet.next(), AdminRequestHandler.start(), ChannelHandlerRequest.send()) against every DriverChannel.write() caller and every new cancel site: the 1:1 pairing holds, I found no double-release. In particular ChannelPool.next(routingKey, shardSuggestion) only ever calls one ChannelSet.next() per invocation, and both shouldPreAcquireId=false handlers (ReprepareOnUp, CqlPrepareHandler.prepareOnOtherNode) really do sit on a pool-owned reservation.

What I liked

• The writeSubmitted flag is the right idiom, not boilerplate to be simplified into catch (Throwable): setting it before addListener means a throw from addListener won't cancel a reservation the pipeline already owns. Worth keeping as-is.
• The AtomicBoolean CAS in ThrottledAdminRequestHandler is genuinely load-bearing — ConcurrencyLimitingRequestThrottler.register() calls onThrottleReady() synchronously, so a throw there cancels once in onThrottleReady's catch and would cancel again in start()'s catch.
• inFlightCallbacks.remove(nodeResponseCallback) in the continuous handler's finally — easy to miss, and a stale callback there would later get aborted spuriously.
• (result, error) → (preparedId, error) in CqlPrepareHandler de-shadows the result field. Nice drive-by.

Risk

Low: pure accounting, no protocol or wire change, no public API surface (all internal.*), one boolean store on the happy path. The failure mode of getting it wrong is over-release — getInFlight() going negative and a channel accepting more than max_requests_per_connection — and I couldn't construct such a path.

Verdict

Correct as written. The two things I'd want before merge are both documentation: the write() ownership contract and the now-stale preAcquireId() / StreamIdGenerator javadoc, since those comments are the only place the invariant holding all five finally blocks together is written down. Everything else is optional. I filed #980 for the sibling leak on these same paths.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Verdict

Approving. All ten round-1 comments are addressed, including the three documentation items I named as my pre-merge conditions — those comments were the only place the invariant holding the five caller-side finally blocks together was written down, and they now say it.

Re-verified at ef698f9

I re-traced the accounting rather than re-reading the replies:

• Producer/consumer pairing is still 1:1 with the six new cancel sites. ChannelPool.next() yields exactly one ChannelSet.next() on every branch (L178, L197, L204), so there's no double-acquire feeding a double-release.
• write()'s no-throw contract now holds by construction, which is what makes the new Javadoc true rather than aspirational: the only throwing statements sit inside the new try, so writeSubmitted is always true once write() returns.
• The catch in write() cannot over-release. With DefaultWriteCoalescer, InFlightHandler.write() runs from the flusher task on the event loop, never synchronously inside writeAndFlush. With PassThroughWriteCoalescer, Netty's invokeWrite0 converts handler exceptions into a failed promise instead of propagating. So nothing can throw after streamIds.acquire() consumed the id. The realistic throw is RejectedExecutionException from eventLoop.execute() — and there the queued Write is permanently stranded (running stays true), so the id really is unused and cancelling is correct.
• holdsExternalReservation.set(false) before super.start() is load-bearing. Inverting those two lines would double-release, since AdminRequestHandler.start()'s own finally covers everything from that point on. Worth keeping in that order.
• The new continuous test genuinely pins the new branch. verifyNoWrite() is what rules out the finally path at L396 — without it the test would pass either way.
• Also checked: AdminRequestHandler schedules timeoutFuture only in onWriteComplete, so a request sitting in the throttler has no timer that could fire setFinalError behind the reservation, and throttler close() drains via onThrottleFailure, which cancels. And the should_complete_result_if_first_node_replies_immediately rewrite is a literal inlining of Builder.withResponse — no semantic change, it just exposes the handle.

On the one thing you declined

Keeping catch (Throwable) is fine. The reasoning refutes the exact snippet I suggested rather than the general shape — rethrowing without cancelling would also have worked — but the contract you chose instead is the better answer anyway: it's documented on the method, it holds by construction, and should_cancel_pre_acquired_id_when_coalesced_write_throws_error pins it. The reservation is released either way, which was my actual concern. No further action.

Non-blocking

Three notes inline. None needs to happen before merge — two are style/consistency, and one belongs to #980, where I've recorded it. Coverage is unchanged from round 1: the four finally paths Copilot flagged are still untested and, as I said then, I'm not blocking on those; the one branch I did ask for is covered.

CI is green across Build, Unit tests and Full verify on JDK 11 and 17 plus every Scylla and Cassandra IT matrix, and #966 landed on 2026-07-20, so the dependency note in the description is resolved.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/adminrequest/ThrottledAdminRequestHandlerTest.java`:
- Around line 100-110: Update newHandler and the cleanup tests to use a real
pre-acquired reservation: call and retain channel.preAcquireId() before
handler.start(), configure the mock stateful reservation behavior, and assert
cancelPreAcquireId() clears the reservation rather than only verifying
invocation. Add separate coverage for the shouldPreAcquireId=true path if
handler-owned acquisition is required.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Scoping this round

My approve on 07-30 was against ef698f9d. Head is 74327dfa — +854 lines across 13 files newer than that tree. The RequestMessage ownership flag, failPendingWrites(), RejectionSafeEventExecutor and the ImmediateEventExecutor switch all landed after it, so none of that machinery carried my review. I re-traced the interesting interleavings (closingGracefully firing the promise inline before cancelPreAcquire(), a stale queued message reaching the pipeline, channel.write() throwing post-submission) and the CAS ownership model holds in all of them — no correctness bug. Approval stands; everything below is docs, dead code, or consistency.

One design question

The delta fixes the same problem twice, two different ways. The problem: an event loop can reject a listener-notification task during shutdown, and netty's DefaultPromise.safeExecute only logs that — so the listener never runs and the request hangs to timeout.

• DefaultWriteCoalescer solves it with a bespoke ~65-line EventExecutor (RejectionSafeEventExecutor, L193-255) that falls back to running listeners on the completing thread.
• DriverChannel.write() solves it by switching its two failure returns to ImmediateEventExecutor (L111, L129).

Both are defensible in isolation; having both, picked inconsistently, is the thing I'd like settled. Passing the flusher's listenerNotificationExecutor into write()'s failed futures would collapse them into one mechanism and drop the ImmediateEventExecutor import. Which is the intended house style here?

Minor: Depends on #966 in the description is stale — #966 merged 2026-07-20, before this PR opened.